Skip to content

OpenApi v3 refactor #3407

Merged
soyuka merged 17 commits into
api-platform:masterfrom
soyuka:openapi
Jul 23, 2020
Merged

OpenApi v3 refactor #3407
soyuka merged 17 commits into
api-platform:masterfrom
soyuka:openapi

Conversation

@soyuka

@soyuka soyuka commented Feb 21, 2020

Copy link
Copy Markdown
Member
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tickets na
License MIT
Doc PR todo

With this rework, I want to improve the developer experience in enhancing our OpenAPI support. By using explicit classes that matches the OpenAPI spec, it should be easier to:

Note that there are some missing features (for example pagination parameters and components) and I wanted to get back some feedbacks before continuing this work as it's a heavy refactor.
I also want to add two new objects (PaginationOptions, OpenApiAuthentificationOptions) to reduce the number of arguments in the constructor.

WDYT ? Should I continue working on this?

@teohhanhui

Copy link
Copy Markdown
Contributor

I think we should make all the classes immutable. Mutability makes the code very hard to reason about, when there's so much going on.

@soyuka

soyuka commented Feb 21, 2020

Copy link
Copy Markdown
Member Author

I think we should make all the classes immutable. Mutability makes the code very hard to reason about, when there's so much going on.

Indeed, many classes aren't good yet there are really at the POC state ^^. I think that the Paths object has to be mutable: indeed it's a unique collection of paths and needs to be query-able by path.

@dunglas

dunglas commented Feb 21, 2020

Copy link
Copy Markdown
Member

Should I continue working on this?

Definitely, it sounds like a great (and needed) design improvement. At some point we'll even maybe be able to provide a standalone OpenAPI component (a subtree split of this repo).

Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/OpenApi/Options.php Outdated
Comment thread src/OpenApi/Options.php Outdated
Comment thread src/OpenApi/Options.php Outdated
Comment thread src/OpenApi/Serializer/OpenApiNormalizer.php Outdated
Comment thread src/OpenApi/Serializer/OpenApiNormalizer.php Outdated
@soyuka soyuka force-pushed the openapi branch 5 times, most recently from e3035ba to 1c3c2dd Compare April 29, 2020 09:21
@soyuka soyuka changed the title [WIP] OpenApi v3 refactor OpenApi v3 refactor Jun 12, 2020
@soyuka soyuka mentioned this pull request Jun 26, 2020
Comment thread src/DataProvider/PaginationOptions.php Outdated
Comment thread src/Documentation/Action/DocumentationAction.php Outdated
Comment thread src/Documentation/Action/DocumentationAction.php Outdated
Comment thread src/Documentation/Action/DocumentationAction.php
Comment thread src/Documentation/Documentation.php
Comment thread src/Documentation/DocumentationInterface.php
Comment thread src/OpenApi/Model/Schema.php Outdated
Comment thread tests/OpenApi/Factory/OpenApiFactoryTest.php Outdated

@dunglas dunglas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job! 👏

Comment thread src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated
Comment thread src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated
Comment thread src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated
Comment thread src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated
Comment thread src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated
Comment thread src/OpenApi/Model/License.php Outdated
Comment thread src/OpenApi/Model/Link.php Outdated
Comment thread src/OpenApi/Model/Link.php Outdated
Comment thread src/OpenApi/Model/Parameter.php
Comment thread src/OpenApi/OpenApi.php Outdated
}

/**
* @return array | array

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array | array doesn't look right... maybe array[] was intended? or even array{array, array} (https://phpstan.org/writing-php-code/phpdoc-types#array-shapes)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed it should be array[]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, I see that since then, $schemas has been changed from array to \ArrayObject... so that would give array{array, \ArrayObject} for PHPStan (or at this point maybe just remove this phpDoc?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the return has become useless, see #4199

return 0 === strpos($operation['path'], '/') ? $operation['path'] : '/'.$operation['path'];
}
$path = $this->operationPathResolver->resolveOperationPath($resourceShortName, $operation, $operationType, $operationName);
if ('.{_format}' === substr($path, -10)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @soyuka Does there any way to enable defining '.{_format}' per resource, even ApiPlatform\Core\OpenApi\Model\Paths does not contain a setter function, in case we decide to decorate the OpenapiFactory ? I have a resource that should export pdf and docx.
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants